-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor search #1594
Refactor search #1594
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what I see! It also seems to work as expected when i search for things :)
}) | ||
.then((results) => | ||
results.map((result) => ({ | ||
id: `${result.type}-${result.match.id}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aaah this is so much nicer than the solution I came up with in my useMemberships
hook yesterday.. (i added ids in the store action)
import { SearchResult } from './components/types'; | ||
import { remoteList, RemoteList } from 'utils/storeUtils'; | ||
|
||
export type SearchResultItem = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep i should have done like this as well....
@@ -17,17 +17,28 @@ export function loadListIfNecessary< | |||
remoteList: RemoteList<DataType> | undefined, | |||
dispatch: AppDispatch, | |||
hooks: { | |||
actionOnError?: (err: unknown) => PayloadAction<unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo smart addition! I can see this coming in handy! :D
resultsError: (state, action: PayloadAction<[string, unknown]>) => { | ||
const [query, error] = action.payload; | ||
state.matchesByQuery[query] = remoteList(); | ||
state.matchesByQuery[query].error = error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I have not seen the error part of these in action anywhere else i think, seems like something we want to take into more parts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! I just delayed error handling (lazily) but there was a test here that failed, and I had to decide whether to rip out the test and all the error code, or just implement crude error handling, so I chose the latter. 😊
b269623
into
issue-1542/refactor-to-use-hooks
Description
This PR refactors the search feature to use redux instead of react-query.
Screenshots
None
Changes
search
featureuseSearch()
hookuseSearch()
inSearchDialog
Notes to reviewer
None
Related issues
Related to #1542